Feature: Component Tag by b1ink0 · Pull Request #200 · rtCamp/frappe-ui-react

b1ink0 · 2026-02-05T08:05:51Z

Description

This PR add a new Tag component based on Espresso by Frappe design.

Screenshot/Screencast

Tag.Demo.mp4

Checklist

I have thoroughly tested this code to the best of my abilities.
I have reviewed the code myself before requesting a review.
This code is covered by unit tests to verify that it works as intended.
The QA of this PR is done by a member of the QA team (to be checked by QA).

Copilot

Pull request overview

This PR introduces a new Tag component based on the Timeless Figma design. The component supports different sizes, variants, icons, and both controlled and uncontrolled visibility states with removal functionality.

Changes:

New Tag component with controlled/uncontrolled visibility pattern
Support for prefix icons, customizable suffix icons, and multiple size/variant options
Comprehensive test suite covering removal behavior and disabled states
Storybook documentation with examples for different variants and use cases

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
packages/frappe-ui-react/src/components/tag/types.ts	TypeScript interface defining Tag component props
packages/frappe-ui-react/src/components/tag/tag.tsx	Main Tag component implementation wrapping Button with custom styling
packages/frappe-ui-react/src/components/tag/tests/tag.tsx	Test suite covering rendering, removal, disabled state, and controlled behavior
packages/frappe-ui-react/src/components/tag/tag.stories.tsx	Storybook stories demonstrating different Tag configurations
packages/frappe-ui-react/src/components/tag/index.ts	Component exports
packages/frappe-ui-react/src/components/index.ts	Registration of Tag in main component exports

Copilot · 2026-02-05T08:11:27Z

packages/frappe-ui-react/src/components/tag/types.ts

+  prefixIcon?: React.ComponentType<any>;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  suffixIcon?: React.ComponentType<any>;


The icon prop types use React.ComponentType<any> which is less type-safe than the pattern used in Button component. The Button component uses React.ComponentType<unknown> for its icon props, which is more restrictive and type-safe.

Consider changing from React.ComponentType<any> to React.ComponentType<unknown> to align with the Button component's type safety pattern (packages/frappe-ui-react/src/components/button/types.ts:11-13).

Copilot · 2026-02-05T08:11:27Z

packages/frappe-ui-react/src/components/tag/tag.tsx

+        </button>
+      )}
+      className={clsx(
+        "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!",


The focus styles are hardcoded to gray color scheme (focus:border-gray-900, focus:ring-outline-gray-3) regardless of the variant or potential theme prop. This creates visual inconsistency when using non-gray variants.

The Button component has theme-specific focus classes (packages/frappe-ui-react/src/components/button/button.tsx:60-65). Consider using variant-aware focus styles or ensuring focus styles match the tag's visual appearance.

Suggested change

"focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!",

"focus:border-current focus:outline-none focus:ring-2 focus:ring-current gap-1.25! cursor-auto!",

Copilot · 2026-02-05T08:11:28Z

packages/frappe-ui-react/src/components/tag/tests/tag.tsx

+describe("Tag Component", () => {
+  it("renders with label", () => {
+    render(<Tag label="Test Tag" />);
+    expect(screen.getByText("Test Tag")).toBeInTheDocument();
+  });
+
+  it("renders prefix icon", () => {
+    const PrefixIcon = () => <span data-testid="prefix-icon" />;
+    render(<Tag label="Tag with Icon" prefixIcon={PrefixIcon} />);
+    expect(screen.getByTestId("prefix-icon")).toBeInTheDocument();
+  });
+
+  it("handles uncontrolled removal", () => {
+    render(<Tag label="Removable Tag" />);
+    // Expect the label to be present
+    expect(screen.getByText("Removable Tag")).toBeInTheDocument();
+
+    const removeButton = screen.getByLabelText("Remove tag");
+    fireEvent.click(removeButton);
+
+    // After click, component should return null
+    expect(screen.queryByText("Removable Tag")).not.toBeInTheDocument();
+  });
+
+  it("handles controlled removal", () => {
+    const handleVisibleChange = jest.fn();
+    const handleRemove = jest.fn();
+
+    const { rerender } = render(
+      <Tag
+        label="Controlled Tag"
+        visible={true}
+        onVisibleChange={handleVisibleChange}
+        onRemove={handleRemove}
+      />
+    );
+
+    const removeButton = screen.getByLabelText("Remove tag");
+    fireEvent.click(removeButton);
+
+    expect(handleVisibleChange).toHaveBeenCalledWith(false);
+    expect(handleRemove).toHaveBeenCalled();
+
+    rerender(
+      <Tag
+        label="Controlled Tag"
+        visible={false}
+        onVisibleChange={handleVisibleChange}
+        onRemove={handleRemove}
+      />
+    );
+    expect(screen.queryByText("Controlled Tag")).not.toBeInTheDocument();
+  });
+
+  it("respects disabled state", () => {
+    const handleRemove = jest.fn();
+    render(<Tag label="Disabled Tag" disabled onRemove={handleRemove} />);
+
+    const removeButton = screen.getByLabelText("Remove tag");
+    expect(removeButton).toBeDisabled();
+
+    fireEvent.click(removeButton);
+    expect(handleRemove).not.toHaveBeenCalled();
+  });
+
+  it("renders with custom class name", () => {
+    render(<Tag label="Custom Class" className="my-custom-class" />);
+    const tagText = screen.getByText("Custom Class");
+    // The button that contains the text
+    const button = tagText.closest("button");
+    expect(button).toHaveClass("my-custom-class");
+  });
+});


The test suite is missing coverage for the suffixIcon prop. While the default X icon is tested implicitly through the remove functionality, there's no test verifying that a custom suffix icon can be provided and rendered correctly.

Add a test case that provides a custom suffixIcon component and verifies it renders instead of the default X icon.

Copilot · 2026-02-05T08:11:28Z

packages/frappe-ui-react/src/components/tag/tests/tag.tsx

+describe("Tag Component", () => {
+  it("renders with label", () => {
+    render(<Tag label="Test Tag" />);
+    expect(screen.getByText("Test Tag")).toBeInTheDocument();
+  });
+
+  it("renders prefix icon", () => {
+    const PrefixIcon = () => <span data-testid="prefix-icon" />;
+    render(<Tag label="Tag with Icon" prefixIcon={PrefixIcon} />);
+    expect(screen.getByTestId("prefix-icon")).toBeInTheDocument();
+  });
+
+  it("handles uncontrolled removal", () => {
+    render(<Tag label="Removable Tag" />);
+    // Expect the label to be present
+    expect(screen.getByText("Removable Tag")).toBeInTheDocument();
+
+    const removeButton = screen.getByLabelText("Remove tag");
+    fireEvent.click(removeButton);
+
+    // After click, component should return null
+    expect(screen.queryByText("Removable Tag")).not.toBeInTheDocument();
+  });
+
+  it("handles controlled removal", () => {
+    const handleVisibleChange = jest.fn();
+    const handleRemove = jest.fn();
+
+    const { rerender } = render(
+      <Tag
+        label="Controlled Tag"
+        visible={true}
+        onVisibleChange={handleVisibleChange}
+        onRemove={handleRemove}
+      />
+    );
+
+    const removeButton = screen.getByLabelText("Remove tag");
+    fireEvent.click(removeButton);
+
+    expect(handleVisibleChange).toHaveBeenCalledWith(false);
+    expect(handleRemove).toHaveBeenCalled();
+
+    rerender(
+      <Tag
+        label="Controlled Tag"
+        visible={false}
+        onVisibleChange={handleVisibleChange}
+        onRemove={handleRemove}
+      />
+    );
+    expect(screen.queryByText("Controlled Tag")).not.toBeInTheDocument();
+  });
+
+  it("respects disabled state", () => {
+    const handleRemove = jest.fn();
+    render(<Tag label="Disabled Tag" disabled onRemove={handleRemove} />);
+
+    const removeButton = screen.getByLabelText("Remove tag");
+    expect(removeButton).toBeDisabled();
+
+    fireEvent.click(removeButton);
+    expect(handleRemove).not.toHaveBeenCalled();
+  });
+
+  it("renders with custom class name", () => {
+    render(<Tag label="Custom Class" className="my-custom-class" />);
+    const tagText = screen.getByText("Custom Class");
+    // The button that contains the text
+    const button = tagText.closest("button");
+    expect(button).toHaveClass("my-custom-class");
+  });
+});


The test suite doesn't verify that different size and variant props are applied correctly. While the stories demonstrate different sizes and variants visually, there are no automated tests ensuring these props result in the correct CSS classes being applied.

Add test cases for different sizes (sm, md, lg) and variants (solid, subtle, outline, ghost) to verify the component renders with the expected classes, similar to the Badge component tests (packages/frappe-ui-react/src/components/badge/tests/badge.tsx:29-52).

Copilot · 2026-02-05T08:11:28Z

packages/frappe-ui-react/src/components/tag/tag.stories.tsx

+import type { Meta, StoryObj } from "@storybook/react-vite";
+import { Plus } from "lucide-react";
+
+import Tag from "./tag";
+import type { TagProps } from "./types";
+
+export default {
+  title: "Components/Tag",
+  component: Tag,
+  tags: ["autodocs"],
+  argTypes: {
+    className: {
+      control: "text",
+      description: "Additional CSS classes for the tag",
+    },
+    size: {
+      control: { type: "select", options: ["sm", "md", "lg"] },
+      description: "Size of the tag",
+    },
+    variant: {
+      control: {
+        type: "select",
+        options: ["solid", "subtle", "outline", "ghost"],
+      },
+      description: "Variant style of the tag",
+    },
+    label: {
+      control: "text",
+      description: "Text label displayed inside the tag",
+    },
+    disabled: {
+      control: "boolean",
+      description: "Disables the tag when set to true",
+    },
+    prefixIcon: {
+      control: false,
+      description: "Icon component displayed before the label",
+    },
+    suffixIcon: {
+      control: false,
+      description: "Icon component displayed after the label",
+    },
+    visible: {
+      control: "boolean",
+      description: "Controls the visibility of the tag (controlled mode)",
+    },
+    onVisibleChange: {
+      action: "visibility changed",
+      description: "Callback function when the visibility of the tag changes",
+    },
+    onRemove: {
+      action: "removed",
+      description: "Callback function when the remove icon is clicked",
+    },
+  },
+  parameters: { docs: { source: { type: "dynamic" } }, layout: "centered" },
+} as Meta<typeof Tag>;
+
+type Story = StoryObj<TagProps>;
+
+export const Default: Story = {
+  args: {
+    size: "sm",
+    variant: "solid",
+    label: "Discover",
+  },
+  render: (args) => (
+    <div className="min-h-20 flex justify-center items-center">
+      <Tag {...args} />
+    </div>
+  ),
+};
+
+export const Subtle: Story = {
+  args: {
+    size: "sm",
+    variant: "subtle",
+    label: "Discover",
+  },
+  render: (args) => (
+    <div className="min-h-20 flex justify-center items-center">
+      <Tag {...args} />
+    </div>
+  ),
+};
+
+export const Outline: Story = {
+  args: {
+    size: "sm",
+    variant: "outline",
+    label: "Discover",
+  },
+  render: (args) => (
+    <div className="min-h-20 flex justify-center items-center">
+      <Tag {...args} />
+    </div>
+  ),
+};
+
+export const Ghost: Story = {
+  args: {
+    size: "sm",
+    variant: "ghost",
+    label: "Discover",
+  },
+  render: (args) => (
+    <div className="min-h-20 flex justify-center items-center">
+      <Tag {...args} />
+    </div>
+  ),
+};
+
+export const Disabled: Story = {
+  args: {
+    size: "md",
+    variant: "solid",
+    label: "Discover",
+    disabled: true,
+  },
+  render: (args) => (
+    <div className="min-h-20 flex justify-center items-center">
+      <Tag {...args} />
+    </div>
+  ),
+};
+
+export const WithPrefix: Story = {
+  args: {
+    size: "md",
+    variant: "solid",
+    label: "Mobile",
+    prefixIcon: () => <Plus className="w-3 h-3" />,
+  },
+  render: (args) => (
+    <div className="min-h-20 flex justify-center items-center">
+      <Tag {...args} />
+    </div>
+  ),
+};


The stories don't include an example of the controlled mode (using the visible and onVisibleChange props). While the tests verify controlled behavior, a story demonstrating this pattern would be helpful for documentation and visual verification.

Consider adding a story that shows controlled tag removal, similar to how the tests demonstrate it (packages/frappe-ui-react/src/components/tag/tests/tag.tsx:29-57).

Copilot · 2026-02-05T08:11:28Z

packages/frappe-ui-react/src/components/tag/tag.tsx

+        </button>
+      )}
+      className={clsx(
+        "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!",


The Tag applies cursor-auto! to make the main tag area non-clickable, which is correct for a tag component. However, this conflicts with the Button component's default cursor styling and may create confusion.

Since the Tag's main body shouldn't be clickable (only the remove button should be), consider whether wrapping a Button component is the right approach. A more semantic implementation might use a span or div with button-like styling for the tag body, and a separate button only for the remove action. This would also resolve the nested button accessibility issue.

Copilot · 2026-02-05T08:11:29Z

packages/frappe-ui-react/src/components/tag/tag.tsx

+    <Button
+      size={size}
+      variant={variant}
+      label={label}
+      iconLeft={prefixIcon}
+      iconRight={() => (
+        <button
+          type="button"
+          className={clsx(
+            "flex items-center justify-center",
+            disabled && "cursor-auto!"
+          )}
+          onClick={handleRemove}
+          disabled={disabled}
+          aria-label="Remove tag"
+        >
+          <SuffixIcon
+            className="w-3 h-3"
+            aria-hidden="true"
+            focusable="false"
+          />
+        </button>
+      )}
+      className={clsx(
+        "focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!",
+        size === "sm" && "text-xs! h-5! rounded-[5px]! px-1.5! py-0.75!",
+        size === "md" && "text-sm! h-6! rounded-[6px]! px-1.5! py-1!",
+        size === "lg" && "text-base! h-7! rounded-[8px]! px-2! py-1.5!",
+        className
+      )}
+      disabled={disabled}
+    />


The current implementation creates nested buttons, which is invalid HTML and causes accessibility issues. The Tag wraps a Button component and passes a button element as iconRight, resulting in a button nested inside another button.

Consider one of these alternatives:

Render the Tag as a div or span container with separate button-like styling, instead of wrapping an actual Button component

Pass a clickable icon/div to iconRight instead of a button element

Restructure the component to not use Button as a wrapper

The nested button structure will fail HTML validation and can cause unpredictable behavior with screen readers and keyboard navigation.

Copilot · 2026-02-05T08:11:29Z

packages/frappe-ui-react/src/components/tag/types.ts

+export interface TagProps {
+  size?: "sm" | "md" | "lg";
+  variant?: ButtonVariant;
+  label?: string;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  prefixIcon?: React.ComponentType<any>;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  suffixIcon?: React.ComponentType<any>;
+  className?: string;
+  disabled?: boolean;
+  visible?: boolean;
+  onVisibleChange?: (visible: boolean) => void;
+  onRemove?: () => void;
+}


The Tag component is missing the theme prop that the Button component supports. The Button component accepts a theme prop (gray, blue, green, red) which controls the color scheme. Without exposing this prop, Tag users cannot customize the color theme of their tags, limiting the component's flexibility.

Add a theme prop to TagProps that accepts ButtonTheme values and pass it through to the Button component.

Add Tag component

bcb90f1

b1ink0 marked this pull request as ready for review February 5, 2026 08:06

b1ink0 requested a review from Copilot February 5, 2026 08:06

Copilot started reviewing on behalf of b1ink0 February 5, 2026 08:06 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

b1ink0 mentioned this pull request Feb 5, 2026

Feature: Add ButtonGroup, RadioButton, Tag, Spacer and Slider components based on Espresso by Frappe #107

Closed

4 tasks

ayushnirwal marked this pull request as draft February 5, 2026 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Feature: Component Tag#200

Feature: Component Tag#200
b1ink0 wants to merge 1 commit intodevelopfrom
feat/component-tag

b1ink0 commented Feb 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	"focus:border-gray-900 focus:outline-none focus:ring-2 focus:ring-outline-gray-3 gap-1.25! cursor-auto!",
	"focus:border-current focus:outline-none focus:ring-2 focus:ring-current gap-1.25! cursor-auto!",

Comments

Conversation

b1ink0 commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot/Screencast

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

b1ink0 commented Feb 5, 2026 •

edited

Loading